-
Notifications
You must be signed in to change notification settings - Fork 2.6k
refactor: unify HistoryView and HistoryPreview with TaskItem to consistently use the existing HistoryItem interface #4019
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
794de76 to
d208854
Compare
Introduces `TaskItem.tsx` and `TaskItemHeader.tsx` to centralize and standardize the rendering of history entries. `TaskItem` handles the overall structure for "compact" (Preview) and "full" (HistoryView) variants. `TaskItemHeader` consolidates all metadata (timestamp, tokens, cost, cache, file size) into a single, consistent line above the task content, enhancing visual clarity and reducing UI clutter. This refactor significantly simplifies `HistoryPreview.tsx` and `HistoryView.tsx`. Approximately 314 lines of previous rendering logic were removed from these components and replaced by 242 lines in the new, focused, and reusable `TaskItem` and `TaskItemHeader` components, resulting in a net reduction and improved maintainability. Most importantly, rendering logic happens in one place. Key UI Changes: - Metadata (timestamp, tokens, cost, cache, file size) now displayed inline on a single header row in both variants. - Removed explicit "Tokens:" and "API Cost:" labels for a cleaner look. - Action buttons (Copy, Export, Delete) in the full view are now aligned with the metadata header. - File size is displayed in the header for the "full" variant only. - Workspace information is no longer displayed in the "compact" preview. Component Changes: - Created `webview-ui/src/components/history/TaskItem.tsx` (125 lines) - Created `webview-ui/src/components/history/TaskItemHeader.tsx` (117 lines) - Modified `webview-ui/src/components/history/HistoryPreview.tsx` (-65 lines, +3 lines) - Modified `webview-ui/src/components/history/HistoryView.tsx` (-249 lines, +3 lines) - Uses `HistoryItem` type for standardized data handling. Fixes: #4018 Signed-off-by: Eric Wheeler <[email protected]>
Fixes test failures that occurred after the major refactoring that introduced the shared TaskItem component. The original implementation was correct but the tests needed updates to match the new component structure. - Add data-testid attributes to TaskItemHeader for reliable test selection - Update TaskItem.test.tsx assertions to use new test IDs for tokens/cache - Fix Checkbox import path in TaskItem.tsx (ui/checkbox vs ui) - Add missing mocks for lucide-react and Checkbox in HistoryView.test.tsx - Update HistoryView test assertions to use correct selectors - Ensure all 19 history component tests pass successfully The refactoring reduced code duplication by ~250+ lines while maintaining functionality, and these test fixes ensure the quality gates remain intact. Signed-off-by: Eric Wheeler <[email protected]>
d208854 to
23b2a8f
Compare
|
@daniel-lxs - This PR blocks further progress on #3785 and #3689. I am not sure how you are managing pull request dependency-order, so FYI in case this is useful. This is ready for review, merging soon would be appreciated so I can continue on the other fronts. |
|
@KJ7LNW Is there a way to make this change without making the change to the UI? |
Create a new TaskItemFooter component that displays token and cost information with different styles for compact and full views. Move the CopyButton and ExportButton from header to footer in full view. Adjust file size display positioning in the header for better visual alignment. Signed-off-by: Eric Wheeler <[email protected]>
|
Someone is attempting to deploy a commit to the Roo Code Team on Vercel. A member of the Team first needs to authorize it. |
Update data-testid attributes in HistoryView and TaskItem tests to match the actual implementation in TaskItemFooter component. The tests were looking for generic "tokens-in" and "tokens-out" attributes, but the implementation uses variant-specific attributes like "tokens-in-footer-full". Also added mocks for CopyButton and ExportButton components to resolve "Element type is invalid" errors during test rendering. Signed-off-by: Eric Wheeler <[email protected]>
|
your changes have been applied so this is ready I'm not sure what the Vercel check does, so hopefully it is not relevant. |
| <i className="codicon codicon-arrow-right" style={metadataIconWithTextAdjustStyle} /> | ||
| <span data-testid="cache-reads">{formatLargeNumber(item.cacheReads || 0)}</span> | ||
| </span> | ||
| )} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @KJ7LNW, I noticed your recent changes are basically moving all the cost/token data back to the bottom.
It seems like the cache information is still on the header next to the timestamp. Is this intentional or was this supposed to be moved as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would love to revert that recent change, but @hannesrudolph was concerned that this would not be accepted if the user interface changed and encouraged me to make it look the same at the cost of slightly more code.
The most recent change creates a <TaskItemFooter> and move things around to look like it did originally, while still maintaining the necessary component organization.
Standardizing all the information on a nice compact header would be great because it shrinks the source code and makes everything consistent, but I am not a user interface person, and the priority for this merge is code organization, not appearance.
This is what it looked like before the change from last night: the left side is current Roo, the right side is what it looked like before I added last night's commits for Hannes:
Either version is fine for me, they both provide the necessary factor. However, the computer scientist in me likes the original because the refactor is much cleaner and the components are standardized.
Please merge this PR in whatever form is most likely to be accepted.
Thanks!
|
@KJ7LNW why is this photo nowhere to be found in this thread? |
See OP, I updated original screen shots last night from our change together, so what it shows at the top is the current PR state. |
|
@KJ7LNW and what about the placement of cached tokens? I don't see an example with that and it seems that that info would show up in the header not where it currently sits. |
|
Closed by #4151 |
…ning. (RooCodeInc#4019) Fix remaining occurences of protobuf literals.


Blocks #3785 #3689
This refactoring is needed by both #3785 and #3689 as they require the standardized structure and consistent use of the existing
HistoryIteminterface across history components.Context
The
HistoryViewandHistoryPreviewcomponents had significant code duplication between compact (preview) and full variants, making maintenance difficult.This PR introduces
TaskItem.tsxandTaskItemHeader.tsxto centralize and standardize the rendering of history entries.TaskItemhandles the overall structure for "compact" (Preview) and "full" (HistoryView) variants.TaskItemHeaderconsolidates all metadata (timestamp, tokens, cost, cache, file size) into a single, consistent line above the task content, enhancing visual clarity and reducing UI clutter.The net change results in fewer lines, and much better organization:
Implementation
UI
Please notice: The intention of this pull request it is to have absolutely no behavior change.
Font sizes have not changed: Visible differences in scale are because my screenshot size differs and Github is scaling funny.
How to Test
Get in Touch
Discord: KJ7LNW
Important
Refactor
TaskItemcomponent to unify compact and full variants, centralizing logic and metadata display, with updated tests to ensure consistent functionality.TaskItem.tsxto unify compact and full variants ofTaskItem.TaskItemHeader.tsxto handle metadata display (timestamp, tokens, cost, cache, file size).TaskItemandTaskItemHeader.HistoryItemtype instead of customTaskItemDatainterface.HistoryView.test.tsxand addsTaskItem.test.tsxto cover new component structure.This description was created by
for 23b2a8f. You can customize this summary. It will automatically update as commits are pushed.